ci: Use Apple Clang 16 to support C++20 on macOS 14; Drop support for Ubuntu 20.04. #29
Conversation
WalkthroughThis pull request updates two GitHub workflow configurations. The code linting checks workflow modifies the operating system specifications, replacing older OS identifiers with new versions, and removes the conditional step for coreutils installation and the logging step. In the unit tests workflow, similar changes to the OS matrix are applied, along with the addition of a conditional step for installing clang-16 on macOS 14 and the removal of certain installation commands for Ubuntu. Changes
Sequence Diagram(s)sequenceDiagram
participant LintJob as Linting Workflow
participant OSMatrix as OS Matrix
participant Coreutils as Coreutils Installer
LintJob->>OSMatrix: Load OS values (macos-15, ubuntu-24.04)
OSMatrix-->>LintJob: Provide updated OS identifiers
LintJob->>Coreutils: Check condition (if OS is macos-15)
Coreutils-->>LintJob: Install coreutils accordingly
sequenceDiagram
participant UTJob as Unit Tests Workflow
participant OSMatrix as OS Matrix
participant CondSteps as Conditional Steps
participant Clang as Clang-16 Installer
participant EnvSetup as Environment Setup
participant TestRunner as Test Executor
UTJob->>OSMatrix: Load OS values (macos-14, macos-15)
OSMatrix-->>UTJob: Return specific OS versions
alt When OS is macos-14
UTJob->>CondSteps: Execute conditional steps for macOS 14
CondSteps->>Clang: Install clang-16 and set compiler env vars
else When OS is macos-15
UTJob->>CondSteps: Execute standard conditional steps for macOS 15
end
UTJob->>EnvSetup: Source .bashrc to load updated env vars
EnvSetup-->>TestRunner: Provide updated environment
UTJob->>TestRunner: Run unit tests
Possibly Related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
requested review from
LinZhihao-723,
davidlion and
kirkrodrigues
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit-tests.yaml (2)
46-55: Unit Test Execution for macOS-14 with Clang-16
This new step properly sets the environment variables (CCandCXX) to point to LLVM@16's AppleClang-16 binaries and installs the required LLVM version using Homebrew before running the tests. One suggestion is to consider caching the LLVM installation (or checking if it is already installed) to avoid redundant installations in repeated runs, which may improve workflow efficiency.
56-60: Simplify the Test Execution Conditional
The conditional expressionif: "false == (matrix.os == 'macos-14')"functions correctly but could be simplified toif: "matrix.os != 'macos-14'"to enhance readability.Would you like a diff suggestion for this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit-tests.yaml(2 hunks)
🔇 Additional comments (2)
.github/workflows/unit-tests.yaml (2)
23-27: Matrix Update for OS Compatibility
The OS matrix now includes"macos-14","macos-15","ubuntu-22.04", and"ubuntu-24.04", which aligns with the updated testing requirements. Verify that these OS versions meet all test expectations across environments.
42-45: Conditional Coreutils Installation
The conditional check for installing coreutils (if: "matrix.os == 'macos-14' || matrix.os == 'macos-15'") is clear and correctly scoped for both macOS environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yaml (1)
57-61: Conditional Expression for Non-macOS-14 Unit TestsThe step for running unit tests on OSes other than macOS-14 uses the condition
"false == (matrix.os == 'macos-14')". While functionally correct, consider refactoring this condition for better readability by using a direct inequality comparison (e.g.,"matrix.os != 'macos-14'").Here's a suggested diff:
- - if: "false == (matrix.os == 'macos-14')" + - if: "matrix.os != 'macos-14')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/code-linting-checks.yaml(1 hunks).github/workflows/unit-tests.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/code-linting-checks.yaml
🔇 Additional comments (3)
.github/workflows/unit-tests.yaml (3)
24-27: OS Matrix Update: New Environment SpecificationsThe updated OS matrix now includes "macos-14", "macos-15", "ubuntu-22.04", and "ubuntu-24.04", which aligns with the new CI requirements outlined in the PR objectives. This ensures testing is performed on the targeted and updated operating systems.
42-47: Dependency Installation on macOS-14: Coreutils and LLVM@16The conditional installation step for macOS-14 now installs both
coreutilsandllvm@16. This is appropriate given the move to AppleClang-16 for C++20 support.Consider verifying that the installation paths and package versions meet pipeline expectations in the CI environment.
48-56: macOS-14 Unit Tests Execution with Clang-16The unit tests step for macOS-14 now correctly sets the environment variables (
CCandCXX) to use the Clang-16 binaries before runningtask cleanandtask test-all. This ensures that tests are compiled with the updated AppleClang-16 supporting C++20.
Bill-hbrhbr
changed the title
MacOS Sonoma to AppleClang-16 support C++20 compilation; Drop Ubuntu-Focal 20.04
Bill-hbrhbr
changed the title
MacOS Sonoma to AppleClang-16 support C++20 compilation; Drop Ubuntu-Focal 20.04MacOS Sonoma to AppleClang-16 support C++20 compilation; Drop Ubuntu-Focal 20.04 and unused GitHub workflow steps
Bill-hbrhbr
changed the title
MacOS Sonoma to AppleClang-16 support C++20 compilation; Drop Ubuntu-Focal 20.04 and unused GitHub workflow stepsMacOS-14 Sonoma to AppleClang-16 support C++20 compilation; Drop Ubuntu-Focal 20.04 and unused GitHub workflow steps
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yaml (1)
49-57: Review YAML Expression in Environment Block.
The multi-line expression for setting environment variables (i.e. overridingCCandCXXfor macOS 14) is innovative; however, actionlint flagged an unexpected "{" character. While GitHub Actions expressions can span multiple lines, consider refactoring the expression into a single line to avoid potential parsing issues. For example:-env: >- - ${{ - 'macos-14' == matrix.os - && fromJson('{ - "CC": "/opt/homebrew/opt/llvm@16/bin/clang", - "CXX": "/opt/homebrew/opt/llvm@16/bin/clang++" - }') - || {} - }} +env: ${{ 'macos-14' == matrix.os && fromJson('{"CC": "/opt/homebrew/opt/llvm@16/bin/clang", "CXX": "/opt/homebrew/opt/llvm@16/bin/clang++"}') || {} }}This simplified format should appease linters like actionlint and ensure correct evaluation.
🧰 Tools
🪛 actionlint (1.7.4)
49-49: got unexpected character '{' while lexing expression, expecting 'a'..'z', 'A'..'Z', '_', '0'..'9', ''', '}', '(', ')', '[', ']', '.', '!', '<', '>', '=', '&', '|', '*', ',', ' '
(expression)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit-tests.yaml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/unit-tests.yaml
49-49: got unexpected character '{' while lexing expression, expecting 'a'..'z', 'A'..'Z', '_', '0'..'9', ''', '}', '(', ')', '[', ']', '.', '!', '<', '>', '=', '&', '|', '*', ',', ' '
(expression)
🔇 Additional comments (3)
.github/workflows/unit-tests.yaml (3)
24-27: Update OS Matrix to Reflect New Platforms.
The matrix now includes "macos-14" and "macos-15" and retains current Ubuntu versions while dropping "ubuntu-20.04". This update aligns with the PR objectives for improved C++20 support on modern systems.
42-47: Scoped Installation for macOS 14 Dependencies.
The installation step correctly applies only when the OS is "macos-14", ensuring thatcoreutilsandllvm@16(for C++20 support) are installed where needed. This targeted approach is appropriate.
58-60: Unit Test Commands Seem Correctly Specified.
The commandstask cleanandtask test-allare clearly delineated under the "Run unit tests" step. They should function as expected within the updated workflow context.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit-tests.yaml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/unit-tests.yaml
48-48: step must run script with "run" section or run action with "uses" section
(syntax-check)
55-55: step must run script with "run" section or run action with "uses" section
(syntax-check)
🔇 Additional comments (2)
.github/workflows/unit-tests.yaml (2)
23-27: Matrix OS List Updated for Target Platforms.
The OS matrix now includes "macos-14", "macos-15", "ubuntu-22.04", and "ubuntu-24.04", in line with the PR objective to drop Ubuntu 20.04 and prepare for AppleClang-16 support for macOS.
42-47: Correct Conditional Installation Step for macOS 14.
The installation step correctly applies for "macos-14" by installing both coreutils and llvm@16 via Homebrew. Ensure that these commands are idempotent so that re-running the workflow does not produce failures if the dependencies are already present.
.github/workflows/unit-tests.yaml
Outdated
| - if: "'macos-14' == matrix.os" | ||
| - name: "Run unit tests" | ||
| env: | ||
| CC: "/opt/homebrew/opt/llvm@16/bin/clang" | ||
| CXX: "/opt/homebrew/opt/llvm@16/bin/clang++" | ||
| run: "task test-all" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Merge the Conditional and Execution Details for macOS-14 Unit Tests.
The current configuration splits the if condition onto a separate step from the unit test execution (name, env, and run). This leads to an invalid step as detected by static analysis (actionlint warning on missing a run or uses key). Please merge the condition with the step so that the step is correctly defined.
Suggested diff:
- - if: "'macos-14' == matrix.os"
- - name: "Run unit tests"
- env:
- CC: "/opt/homebrew/opt/llvm@16/bin/clang"
- CXX: "/opt/homebrew/opt/llvm@16/bin/clang++"
- run: "task test-all"
+ - name: "Run unit tests for macos-14"
+ if: "'macos-14' == matrix.os"
+ env:
+ CC: "/opt/homebrew/opt/llvm@16/bin/clang"
+ CXX: "/opt/homebrew/opt/llvm@16/bin/clang++"
+ run: "task test-all"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - if: "'macos-14' == matrix.os" | |
| - name: "Run unit tests" | |
| env: | |
| CC: "/opt/homebrew/opt/llvm@16/bin/clang" | |
| CXX: "/opt/homebrew/opt/llvm@16/bin/clang++" | |
| run: "task test-all" | |
| - name: "Run unit tests for macos-14" | |
| if: "'macos-14' == matrix.os" | |
| env: | |
| CC: "/opt/homebrew/opt/llvm@16/bin/clang" | |
| CXX: "/opt/homebrew/opt/llvm@16/bin/clang++" | |
| run: "task test-all" |
🧰 Tools
🪛 actionlint (1.7.4)
48-48: step must run script with "run" section or run action with "uses" section
(syntax-check)
.github/workflows/unit-tests.yaml
Outdated
| - if: "false == ('macos-14' == matrix.os)" | ||
| - name: "Run unit tests" | ||
| run: "task test-all" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Merge the Conditional and Execution Details for Non-macOS-14 Unit Tests.
Similarly, the step for executing unit tests on platforms other than "macos-14" is split into a standalone if condition and a separate step. Merge them into one mapping to avoid YAML syntax errors and satisfy the actionlint requirements.
Suggested diff:
- - if: "false == ('macos-14' == matrix.os)"
- - name: "Run unit tests"
- run: "task test-all"
+ - name: "Run unit tests for non-macos-14"
+ if: "false == ('macos-14' == matrix.os)"
+ run: "task test-all"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - if: "false == ('macos-14' == matrix.os)" | |
| - name: "Run unit tests" | |
| run: "task test-all" | |
| - name: "Run unit tests for non-macos-14" | |
| if: "false == ('macos-14' == matrix.os)" | |
| run: "task test-all" |
🧰 Tools
🪛 actionlint (1.7.4)
55-55: step must run script with "run" section or run action with "uses" section
(syntax-check)
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
ci: Use Apple Clang 16 to support C++20 on macOS 14; Drop support for Ubuntu 20.04.
MacOS-14 Sonoma to AppleClang-16 support C++20 compilation; Drop Ubuntu-Focal 20.04 and unused GitHub workflow stepsApple Clang 16 to support C++20 on macOS 14; Drop support for Ubuntu 20.04.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
changed the title
Apple Clang 16 to support C++20 on macOS 14; Drop support for Ubuntu 20.04.
2 participants
Description
AppleClang-15has numerous C++20 bugs that are not fixed untilAppleClang-16, including:TraceableExceptionubuntu-20.04since it's hard to supportC++20on this platform.linux:Ubuntu-24.04 Nobledarwin:MacOS 15 SequoiaNote that we cannot do
because
/usr/binis strictly protected on MacOS.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
Tests
Chores